Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[#1966] Add cypress test for merged groups contribution bars #1970

Merged
merged 7 commits into from
Apr 10, 2023
Merged

[#1966] Add cypress test for merged groups contribution bars #1970

merged 7 commits into from
Apr 10, 2023

Conversation

MarcusTXK
Copy link
Member

Fixes #1959

Proposed commit message

Currently, there are no tests to ensure that the length and the number 
of contribution bars for merged groups are correct. This resulted in 
issue #1959, where the contribution bar went missing, being undetected. 

Let's add a cypress test to check that the length and the number of 
contribution bars for merged groups are correct, even after refresh.

Other information

The cypress test checks if the widths of summary-chart__contrib--bar are 100%, 100% and 50%.
image

Note: This should not be merged before #1960, as the cypress test will fail until the bug fix is merged in, below is the current summary-chart__contrib--bar due to the bug on reload:
image

@MarcusTXK MarcusTXK requested a review from a team April 2, 2023 15:55
@MarcusTXK MarcusTXK force-pushed the 1966-cypress-test-merged-groups branch from c2d2eeb to 1141240 Compare April 2, 2023 15:58
vvidday
vvidday reviewed Apr 3, 2023
View reviewed changes
Copy link
Contributor

@vvidday vvidday left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your work on this @MarcusTXK! The test cases look good overall but I just had a question regarding the hardcoding aspect of it.

frontend/cypress/tests/chartView/chartView_mergeGroup.cy.js Show resolved Hide resolved
@MarcusTXK
Copy link
Member Author

MarcusTXK commented Apr 3, 2023 •
edited
Loading

I have updated the cypress test to include a non-hardcoded version. It now stores the widths and the number of bars, refreshes, and then checks if there are the same number of contribution bars and if they are the same width.

However, I left a test checking if the width and the number of contribution bars are as expected, as I realized that there is currently no test for the contribution bars and I think it would be good to be able to test that the calculation is correct. However, this does require a slight hardcoding.

@HCY123902 HCY123902 requested a review from vvidday April 3, 2023 12:29
vvidday
vvidday reviewed Apr 4, 2023
View reviewed changes
Copy link
Contributor

@vvidday vvidday left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the changes, the tests LGTM! Just thought it would be better to include a small comment about the assumption regarding the hardcoded values, as in other similar test cases: https://github.com/reposense/RepoSense/blob/master/frontend/cypress/tests/zoomView/zoomView_selectFileTypes.cy.js#L52

frontend/cypress/tests/chartView/chartView_mergeGroup.cy.js Show resolved Hide resolved
@MarcusTXK @vvidday
Update test to include assumption
29656f3 
Co-authored-by: David Ong <45852430+vvidday@users.noreply.github.com>
@MarcusTXK MarcusTXK requested a review from a team April 4, 2023 14:58
@vvidday vvidday self-requested a review April 4, 2023 15:06
vvidday
vvidday approved these changes Apr 4, 2023
View reviewed changes
Copy link
Contributor

@vvidday vvidday left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@HCY123902 HCY123902 requested a review from a team April 4, 2023 15:16
@MarcusTXK MarcusTXK force-pushed the 1966-cypress-test-merged-groups branch from 8747a2c to 461e1d2 Compare April 6, 2023 06:21
@MarcusTXK MarcusTXK changed the title [1966] Add cypress test for merged groups contribution bars [#1966] Add cypress test for merged groups contribution bars Apr 6, 2023
zhoukerrr
zhoukerrr approved these changes Apr 7, 2023
View reviewed changes
Copy link
Contributor

@zhoukerrr zhoukerrr left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. More testings are always good

@dcshzj dcshzj merged commit 53d5197 into reposense:master Apr 10, 2023
@github-actions
Copy link
Contributor

The following links are for previewing this pull request:

@MarcusTXK MarcusTXK mentioned this pull request Apr 14, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vvidday vvidday vvidday approved these changes

@zhoukerrr zhoukerrr zhoukerrr approved these changes

@HCY123902 HCY123902 HCY123902 approved these changes

Assignees
No one assigned
Labels
s.ToMerge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Missing contribution bar for merged groups after refresh
5 participants
@MarcusTXK @vvidday @zhoukerrr @HCY123902 @dcshzj